Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: replace common.fixturesDir with common.fixtures #15895

Closed
wants to merge 1 commit into from
Closed

test: replace common.fixturesDir with common.fixtures #15895

wants to merge 1 commit into from

Conversation

tkg214
Copy link
Contributor

@tkg214 tkg214 commented Oct 6, 2017 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@mscdex mscdex added the https Issues or PRs related to the https subsystem. label Oct 6, 2017
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
BridgeAR
BridgeAR reviewed Oct 9, 2017
View reviewed changes
test/parallel/test-https-pfx.js Outdated
@@ -28,7 +28,7 @@ const assert = require('assert');
const fs = require('fs');
const https = require('https');

const pfx = fs.readFileSync(`${common.fixturesDir}/test_cert.pfx`);
const pfx = fs.readFileSync(`${common.fixtures}/test_cert.pfx`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the task description was probably not clear enough. I am not sure if this would actually pass or not but the idea was to require the ../common/fixtures and to use the path function from that module instead of using the more verbose version here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tkg214 thank you very much for participating in the code-and-learn. As @BridgeAR indicates, there are some issues with the changes. This should be:

const pfx = fixtures.readSync('test_cert.pfx');

BridgeAR
BridgeAR previously requested changes Oct 9, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just marking this so it does not get landed until this is fixed.

@BridgeAR
Copy link
Member

Ping @tkg214

@apapirovski
Copy link
Member

Hi @tkg214, would you like to follow up on the requested changes so this can land? Let me know if any clarifications are needed. Thanks for helping us improve Node!

@Trott Trott force-pushed the patch-test-first-commit branch from e55f865 to 40b8259 Compare October 28, 2017 02:04
@Trott
Copy link
Member

Trott commented Oct 28, 2017

I went ahead and fixed this up per @jasnell's suggestion (and the original intention of the task). Hope that's OK.

This should be good to land. PTAL @BridgeAR

CI: https://ci.nodejs.org/job/node-test-pull-request/11034/

joyeecheung
joyeecheung approved these changes Oct 28, 2017
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, CI failures look unrelated

@gireeshpunathil
Copy link
Member

ping @BridgeAR again to have another look ( one of 3 remaining code-and-learn PRs!)

@Trott Trott dismissed BridgeAR’s stale review November 1, 2017 03:53

Requested changes have been made.

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 1, 2017
@t-k-g @Trott
test: use fixtures module in test-https-pfx
c52fe67 
PR-URL: nodejs#15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 1, 2017

Landed in c52fe67.

Thanks for the contribution! 🎉

@Trott Trott closed this Nov 1, 2017
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@t-k-g @Qard
test: use fixtures module in test-https-pfx
83a0540 
PR-URL: nodejs/node#15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@t-k-g @Qard
test: use fixtures module in test-https-pfx
1f19d4d 
PR-URL: nodejs/node#15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
@t-k-g @cjihrig
test: use fixtures module in test-https-pfx
f9b2099 
PR-URL: nodejs#15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 14, 2017
@t-k-g @MylesBorins
test: use fixtures module in test-https-pfx
9416dab 
PR-URL: #15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
@t-k-g @gibfahn
test: use fixtures module in test-https-pfx
2f66faf 
PR-URL: #15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
Merged
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@t-k-g @addaleax
test: use fixtures module in test-https-pfx
f8f2ebc 
PR-URL: nodejs/node#15895
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@watilde watilde watilde approved these changes

@BridgeAR BridgeAR BridgeAR left review comments

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.